Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preferences: Add option to control preloading of video data #4122

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Preferences: Add option to control preloading of video data #4122

merged 3 commits into from
Oct 8, 2024

Conversation

Nerdmind
Copy link
Contributor

@Nerdmind Nerdmind commented Sep 26, 2023 •
edited by SamantazFox
Loading

This PR fixes #4110 by adding a configuration option to control the preloading of video data on page load by making use of the HTML5 preload attribute of the <video> element.

The option is enabled by default, so the preload attribute will have the default value auto. If users want to prevent preloading of video data, they can change the option to false so that the attribute will have the value none.

@Nerdmind
Add option to control preloading of video data
bf47070 
Fix iv-org#4110 by adding an option to control the preloading of video data on
page load. If disabled ("false"), the browser will not preload any video
data until the user explicitly hits the "Play" button.

If enabled ("true"), the default behavior will be used, which means the
browser decides how much of the video will be preloaded.
syeopite
syeopite reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@syeopite syeopite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason why you're having the preload parameter directly set the videojs option?

src/invidious/views/components/player.ecr Outdated Show resolved Hide resolved
assets/js/player.js Outdated Show resolved Hide resolved
@Nerdmind
Copy link
Contributor Author

Nerdmind commented Sep 27, 2023 •
edited
Loading

Hi. No, not a specific reason. But your question got me thinking about something else:

I've set the preload option in VideoJS explicitly because preload: "auto" is currently defined in the player.js file in the master branch and I didn't consider that this option could be removed completely from the player.js when the HTML <video> element already contains the preload attribute directly so that VideoJS can inherit it.

I just tested it locally when removing setting the preload option in player.js completely and it works as expected. I will add a new commit to this PR in a few minutes. This seems to be a cleaner solution with less redundancy.

@Nerdmind
Don't redefine the "preload" option in player.js
824cc1a 
If the HTML5 "<video>" element defines the "preload" attribute directly,
it isn't necessary to redefine the "preload" option in the player.js.
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Dec 28, 2023
@syeopite syeopite removed the stale label Dec 28, 2023
@Nerdmind
Copy link
Contributor Author

Nerdmind commented Mar 8, 2024

I just want to inform you that I'm actively using this feature on my private instance since about half a year now. It works flawlessly and never made a problem. Any chance this PR will get reviewed/merged soon?

@TheFrenchGhosty TheFrenchGhosty requested review from syeopite and SamantazFox and removed request for SamantazFox March 8, 2024 22:50
@syeopite
Copy link
Member

syeopite commented Mar 8, 2024

@SamantazFox Any chance this can be merged in the next batch?

@github-actions github-actions bot added the stale label Jul 14, 2024
@iv-org iv-org deleted a comment from github-actions bot Jul 14, 2024
@unixfox unixfox removed the stale label Jul 14, 2024
@syeopite syeopite added the ready label Aug 30, 2024
@Nerdmind @SamantazFox
Update config/config.example.yml
f247b2f 
Accept suggested change from @SamantazFox.

Co-authored-by: Samantaz Fox <coding@samantaz.fr>
@SamantazFox SamantazFox changed the title Add configuration option to control preloading of video data Preferences: Add option to control preloading of video data Oct 8, 2024
@SamantazFox SamantazFox merged commit 82ac9a8 into iv-org:master Oct 8, 2024
@SamantazFox
Copy link
Member

Thanks for your contribution to invidious :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamantazFox SamantazFox SamantazFox approved these changes

@unixfox unixfox unixfox approved these changes

@TheFrenchGhosty TheFrenchGhosty Awaiting requested review from TheFrenchGhosty

@syeopite syeopite Awaiting requested review from syeopite

Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] Add config option like "Don't autoload video data"
4 participants
@Nerdmind @syeopite @SamantazFox @unixfox